-
-
Notifications
You must be signed in to change notification settings - Fork 68
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Addresses #131. Added check to see if TTY is available. #135
Conversation
Looks great to me, quick eyeball on mobile. Will come back and merge probably on Sunday |
I almost made this exact same PR earlier this afternoon... The irony is this is fixed in symfony/process:^4.1. |
Bump, ran into this recently. I'd rather not pin on an older version. |
Alternatively, as the prefered install seems to be via phar, just change the composer dependencies to:
|
@heddn I don't think that works with Drupal projects though since drupal/core requires symfony/process as ~3.4.0. EDIT: Right, you said phar, nevermind about the above then if you're using a phar :) For those who aren't using a phar though... it's tough luck until this is merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
Could we tag a new release so this gets picked up by my CI? |
Yes please for a release! We'd like to use https://www.drupal.org/project/upgrade_status, but it needs 0.12 of phpstan/phpstan-deprecation-rules, but we use BLT which pins drupal-check By the way, can #131 be closed now this is merged? Thank you! |
I kept the issue open to remind me to tag the release 😬 |
1.1.1 is out :) thanks for all of the patience |
Not sure if this is the kind of fix you had mind for #131 or not, @mglaman, but I copied the
Tty
class from consolidation/site-process referenced by @danepowell in #131 (comment)